sso-proxy: avoid oversized cookies #95
Conversation
|
Thanks for opening this PR @sporkmonger. Could you add some tests and also add the server-side error for exceeded payloads in this PR if it's not too much? |
|
Sure, not a problem. Might take a couple days since I've got a bunch of stuff in the air right now. |
|
saw something that might be on point; https://hajekj.net/2017/03/20/cookie-size-and-cookie-authentication-in-asp-net-core/ |
|
@sporkmonger I have a potential solution for supporting multiple cookies in a session to store the session state but it depends on #43 being completed. I have this prioritized to work on this week, so I'll try to push up a PR as soon as I can. After both |
mreiferson
changed the title
|
@shrayolacrayon now that #43 / #137 are done, could you elaborate a bit on the multi-cookie idea? |
sporkmonger
force-pushed
the
compress-cookie
branch
from
c4d5578 to
b12c1b4
Compare
|
I've made significant progress on a cookie-spanning PR, but I'm going to implement it separately from this one. ☝️ I talked to @shrayolacrayon to confirm no issue w/ plans. |
|
This PR does not interfere with #150. We're running a branch that has both. |
|
@sporkmonger, I approved this but it looks like there might be a CI failure, probably from updating the stale branch. I can merge this in once that's resolved! |
|
OK, I'll take a look. |
Problem
Some potential providers (e.g. Azure AD) use very large tokens. Once you apply encryption to them, they exceed 4096 bytes, which causes the browser to reject them. This PR gives a little more breathing room for larger tokens.
Solution
This gzips the marshalled json before encrypting it. I couldn't think of an obvious scenario where introducing compression would leak information (via cookie length) that would compromise security since the contents of the cookie are already largely known to the client, and this doesn't aid in forging an attacker controlled cookie.
Notes
I was getting cookies in the ballpark of 4400 bytes before this PR, and they're running around 2900 bytes with the same payload now. Chrome (and presumably most other browsers) enforces a limit of 4096 bytes. May be worth also triggering a server side error if we exceed that, since it took me 3 days of debugging to figure out that was happening.